Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add new assertVisual command #2078

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add new assertVisual command #2078

wants to merge 1 commit into from
+112 −0

Conversation

TheKohan
Copy link

@TheKohan TheKohan commented Oct 7, 2024 •
edited
Loading

Proposed changes

This PR is built on top of #1898 by @bartekpacia and implements a basic assertVisual flow which is much awaited in the community (#1222).

Given a simple React Native app

app.tsx

export default function App() {
  const [toggle, setToggle] = React.useState(false);
  return (
    <View style={styles.container}>
      <View style={[styles.dot, {backgroundColor: toggle ? 'red' : 'blue'}]}>
        <Button title="test" onPress={() => setToggle(!toggle)}></Button>
      </View>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    alignItems: 'center',
    marginTop: 200,
  },
  dot: {
    backgroundColor: 'red',
    padding: 30,
    borderRadius: '50%',
  },
});

and a flow

test-flow.yml

appId: com.example.app
---
- launchApp
- tapOn: 'test'
- assertVisual: Login screen # compares the current screen against `.maestro/visual_regression/Login screen.png`

maestro will compare screenshots, producing result image with differences masking.

 ║  > Flow: test-flow                                             
 ║                                                                
 ║    ✅   Launch app "com.example.app"                                                              
 ║    ✅   Tap on "test"                                          
 ║    ✅   Tap on "test"                                          
 ║    ❌   Assert visual difference with baseline Login screen (threshold: 95%)                                                     
 ║
Baseline Current Result
Login screen image Login screen

Result for now is being saved in .maestro/failed_visual_regression/ directory.

If there's no baseline, current will be saved as one.

What is missing:

  • changes accepting flow
  • cloud integration
  • testing
  • remove system-ui elements (like status-bar)
  • component-scoped tests (partial screenshots for component of given test-id)

Testing

  • in progress - test plan has not yet been laid out due to potentially big changes in the proposed flow.

Issues fixed

@TheKohan TheKohan mentioned this pull request Oct 7, 2024
Open
@Fishbowler
Copy link
Contributor

You've mentioned missing the "changes accepting flow". What have you got in mind for the workflow for a failure where the new "look" is as-intended?

@TheKohan
Copy link
Author

TheKohan commented Oct 8, 2024

You've mentioned missing the "changes accepting flow". What have you got in mind for the workflow for a failure where the new "look" is as-intended?

@Fishbowler - the only thing i could've think of as of right now is to keep baseline in repository and in case of expected diff fail CI with a message and let the user manually delete current baseline. #1222 - here are some takes on this problem also.

@jtdaugh
Copy link

jtdaugh commented Oct 8, 2024

You've mentioned missing the "changes accepting flow". What have you got in mind for the workflow for a failure where the new "look" is as-intended?

@Fishbowler - the only thing i could've think of as of right now is to keep baseline in repository and in case of expected diff fail CI with a message and let the user manually delete current baseline. #1222 - here are some takes on this problem also.

We would use this asap in our project and for us modifying the "baseline" image as part of the PR would be our expected workflow, and would actually be a great way for a design/product stakeholder to review any of the baseline screenshot diffs as part of a PR. In other words, no additional build here at all is a perfectly good "accept changes" workflow for our team.

Fishbowler
Fishbowler approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not staff, but this LGTM. Awesome, in fact!

I've left a couple of comments, but neither is a hill I'll die on.

Would need an accompanying PR in the docs repo, which we'd probably want to label with "experimental" or something at this point.

maestro-orchestra-models/src/main/java/maestro/orchestra/Commands.kt
override val label: String? = null,
) : Command {
override fun description(): String {
return label ?: "Assert visual difference with baseline $baseline (threshold: $thresholdPercentage%)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nitpick, I know, but thinking of terminal in a vscode window on a MacBook... Could this be shorter?

Perhaps not 🤔

Copy link
Contributor

@bartekpacia bartekpacia Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideas:

  • visual difference -> visual diff maybe?
  • display just filename of $baseline, not the whole path

maestro-orchestra/src/main/java/maestro/orchestra/yaml/YamlAssertVisual.kt
import com.fasterxml.jackson.annotation.JsonCreator
import java.lang.UnsupportedOperationException

private const val DEFAULT_DIFF_THRESHOLD = 95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be exposed as an environment variable (assuming you don't want to go as far as to expose it to workspace config at this point)?

@TheKohan TheKohan marked this pull request as ready for review October 9, 2024 06:33
@TheKohan TheKohan changed the title feat: add assertVisual command draft feat: add new assertVisual command Oct 9, 2024
@TheKohan TheKohan mentioned this pull request Oct 9, 2024
Open
@TheKohan
Copy link
Author

@bartekpacia - would gladly accept review/guidance on this one. Also #2086 would be invaluable for this.

@rikur
Copy link

rikur commented Oct 18, 2024 •
edited
Loading

Excellent to see some new features coming into Maestro, thank you @TheKohan 🙏

Could mix very well with this: #1526

@bartekpacia
Copy link
Contributor

Hey @TheKohan – this is some exciting work. I no longer work at @mobile-dev-inc, and am a bit sad I didn't have time to finish this:(

cc @amanjeetsingh150 – he's the right person to review this PR:)

@TheKohan
Copy link
Author

@amanjeetsingh150 - can you review this PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartekpacia bartekpacia bartekpacia left review comments

@Fishbowler Fishbowler Fishbowler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TheKohan @Fishbowler @jtdaugh @rikur @bartekpacia